Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #27353 - use vmware network id instead of name #6918

Merged
merged 1 commit into from
Oct 2, 2019

Conversation

ezr-ondrej
Copy link
Member

Use network id, not it's name. As if the name is ambigous, fog-vsphere uses the first one, which can be one we are unable to provision on.

Fog supports it from version 3.1.0, which is already in our packaging.

@theforeman-bot
Copy link
Member

Issues: #27353

@@ -464,7 +464,7 @@ def parse_networks(args)
# Convert network id into name
net = dc_networks.detect { |n| [n.id, n.name].include?(interface['network']) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also be changed to first try all IDs and otherwise fall back to a name? I'm wondering if there's a case where the first network has the ID of a later network and you could end up with a mismatch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the network id should be always unique.
It could happen though in rare cases that you name your network in a manner of the vmware network id (i.e. network_4302) and it could end up selected. I left this for hammer as now if you have in some hammer commands a network name, it works, but it wouldn't now. But we can iterate twice. It will probably be better, to avoid the weird naming usecases...

@ezr-ondrej
Copy link
Member Author

Updated according to the suggestion (first looking by id, than by name).

tbrisker
tbrisker previously approved these changes Sep 16, 2019
Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me but my vmware knowledge is very limited - would feel safer to merge if @timogoebel or @chris1984 can also ack.

chris1984
chris1984 previously approved these changes Sep 16, 2019
Copy link
Member

@chris1984 chris1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good ACK. Only question I had is should we remove this comment since we are using ID now? https://github.com/theforeman/foreman/pull/6918/files#diff-4443c0de1e4e867a30d3c878b9cb64e9R463

@ezr-ondrej
Copy link
Member Author

Looks good ACK. Only question I had is should we remove this comment since we are using ID now? https://github.com/theforeman/foreman/pull/6918/files#diff-4443c0de1e4e867a30d3c878b9cb64e9R463

Removed the comment. Thanks @chris1984.

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ezr-ondrej and @chris1984 !

@tbrisker tbrisker merged commit 97e72f8 into theforeman:develop Oct 2, 2019
@ezr-ondrej ezr-ondrej deleted the vmware_use_id branch October 3, 2019 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants